feat: coalesce concurrent CIMD document fetches for the same client_id - #312
Conversation
The resolution cache only helps once a fetch has COMPLETED. Until then every arriving request was a miss and started its own fetch, so N simultaneous first-time requests for one client_id meant N DNS resolutions, N TLS handshakes, and N x up-to-5s of request occupancy for a document that is byte-identical every time. That multiplier was free to the caller and needed no credential: client resolution runs ahead of the PrincipalResolver chain (#285), so an unauthenticated request reaches the fetch, and concurrency was the only input required. ResolveClient now collapses concurrent misses for the same client_id into a single flight via golang.org/x/sync/singleflight; the rest wait on that result. The fetch/validate/synthesize/cache body moves to resolveUncached so the callback stays readable. Two details that are easy to get wrong: - The flight re-checks the cache before fetching. A fetch may complete between the outer miss and entering the flight, and starting another one would be duplicate work the flight itself cannot see. - Each waiter gets its OWN clone. singleflight hands the same value to every caller, and callers receive a mutable *domain.OAuthClient -- the same reason cachedResult already returns a copy. Sharing one instance would let any waiter mutate what the others hold. Scope, stated plainly because it is easy to over-read: this bounds DUPLICATE CONCURRENT work for one client_id. It does NOT bound distinct-URL abuse -- a caller cycling unique paths gets a fresh flight each time, misses the cache, walks past negative caching, and churns eviction. Only an edge rate limit closes that, which docs/cimd.md now says explicitly rather than as an aside. The test is mutation-checked: bypassing the flight makes it report 8 fetches instead of 1. Its handler blocks until every caller has arrived, so an implementation that serialises rather than coalesces fails too. It also asserts the waiters hold distinct clients and that mutating one caller's RedirectURIs does not change another's. Passes under -race. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
As a couple of side notes... the database, or shared distributed caches are the safe places to store the CIMD Metadata. The reasons is simple - you might have multiple running instances of ZeroID. The CIMD specification, as of the time this comment was written, is currently in its second draft, and my guess is we're going to see changes in it. |
|
Both good calls, thank you — and the first one changed how I'd frame this PR. On the per-instance cacheYou're right, and it's worth putting a number on it: prod AuthN autoscales 2–6 replicas, so One distinction I'd draw before we reach for shared storage, because I think it splits the problem in two: A shared cache fixes fan-out and consistency. It does not fix staleness. Redis or Postgres would make all replicas consistently stale rather than inconsistently stale — nothing about shared storage makes a cached document fresher. The security-relevant property is revocation latency: a client that pulls a compromised So I'd treat them as separate decisions:
Today the inconsistency has a concrete edge: two replicas can serve different versions of one document for up to an hour, so a client's own remediation may or may not have taken effect depending on which pod they land on — and they can't tell which. That's the part I find least comfortable, and notably it's the part shared storage doesn't fix. On medium, if we do go sharedI'd argue Redis over the database, fairly strongly. CIMD's defining property is that nothing is persisted — the synthesized client carries Redis is a much better fit — AuthN already has it for backchannel, quarantine and revocation, so no new dependency. One thing I'd want decided deliberately rather than as a side effect, though: what's being cached is My honest read for right now: at 2–6 replicas the fan-out is a handful of extra fetches per hour, and the abuse case it amplifies is bounded by edge rate limiting on On the draft movingAgreed, and this PR now records where we stand — I've added a "Specification revision and deviations" section to The strictness is the part your comment made me want written down. Rejecting a query string (draft says only SHOULD NOT) and requiring The failure mode I'd flag hardest is the discovery field name. If Two things that already make drift survivable, which I've also written down: DCR is retained as a deliberate fallback, and registry-first resolution means any client caught by a spec change can be pinned by registering it, overriding whatever its document says. |
…cess cache
Follows review on this PR. Two gaps in docs/cimd.md that the reviewer's
questions exposed: nothing recorded WHICH draft revision the
implementation targets, and nothing warned that the resolution cache is
per process.
Adds a "Specification revision and deviations" section covering:
- the revision implemented against
(draft-ietf-oauth-client-id-metadata-document-02, WG-adopted Oct
2025), stated plainly as a draft that will change
- the two places ZeroID is deliberately STRICTER than the draft --
rejecting a query string (draft: SHOULD NOT) and requiring
client_name (draft: RECOMMENDED) -- with the reasoning for each and
an explicit note to revisit both on every draft bump. Strictness is
the part that ages badly: a later revision can bless what we refuse,
and then we reject valid clients for a reason nobody remembers
choosing.
- what is deliberately NOT built (confidential clients via
private_key_jwt + jwks_uri, and software_statement), which are also
the areas the draft is likeliest to move in
- the change most likely to break SILENTLY: a rename of
client_id_metadata_document_supported. Nothing errors -- the server
advertises a key clients no longer look for, they fall back to DCR,
and the flow keeps working via the row-per-client path CIMD exists
to remove. Tests do not help, because they assert the server EMITS
the field; they stay green while no client can see it. Written down
because tracking the draft is the only guard.
- what makes drift survivable: DCR retained as a fallback, and
registry-first resolution as a pinning mechanism for any single
client caught by a spec change.
Adds a deployment note that the cache is per process, since this PR's
singleflight coalesces within a process and not across replicas. Names
both consequences -- fan-out of up to N fetches per document per TTL
with per-replica negative caching, and non-uniform staleness where two
replicas serve different versions of one document for up to the TTL.
States the distinction that matters for anyone reaching for Redis: a
shared cache fixes fan-out and makes replicas CONSISTENTLY stale, but it
does not make them FRESHER. Revocation latency is governed by the TTL
and the document's Cache-Control, wherever the cache lives. Also notes
that a shared cache holds redirect_uris -- the primary anti-impersonation
control -- so write access to it is equivalent to choosing where
authorization codes are delivered.
"Limitations / future work" now links to those sections instead of
restating them, so the two cannot drift apart. All three intra-doc
anchors verified to resolve.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed The PR description overstated what this fixes. It said coalescing bounds duplicate concurrent work, which is true, but it did not say that the bound is per process. With prod AuthN autoscaling 2–6 replicas, the fan-out you identified survives this change entirely — the singleflight collapses concurrent fetches within one process and does nothing across them. The new Specification revision and deviations section records:
I have deliberately not changed the caching design in this PR — it stays the in-process improvement it is, with the limitation documented. If you would rather the inconsistency window be closed before this lands, say so and I will pick up shared caching as its own change with the poisoning question settled first; I do not think it should ride in as a caching detail either way. |
Addresses the Oracle review on #284. ## The vetted bypass depended on an untested call Once `cimd.allowed_domains` names a host, `refusesRedirectTo` stops refusing redirects for self-asserted clients deployment-wide, without re-checking the individual client's redirect host. That is only sound because resolution has already refused any document declaring an off-list https `redirect_uri` -- vetting the publication host alone does not cover it, since a document hosted on an allow-listed domain can declare `redirect_uris` pointing anywhere. `TestRedirectHostsAllowed` covered the predicate in isolation. Nothing covered the wiring, and the wiring is what can vanish: rebasing this branch onto the #312 singleflight refactor moved the fetch into `resolveUncached` and severed that call outright. It surfaced only because it happened to be a compile error. A refactor that left a same-named host variable in scope would have kept compiling while vetting the wrong host, and no test would have failed. `TestCIMDResolveClient_OffListRedirectURIRefused` drives `ResolveClient` end to end: a document served BY the allow-listed host declaring an off-list https redirect_uri must be refused, plus a control proving an allow-listed redirect host still resolves. Verified by mutation -- removing the `redirectHostsAllowed` call makes it fail with its own diagnostic. ## Two loopback classifications were unpinned `RedirectDeliversLocally` is the only gate for an unvetted self-asserted client, so its edge cases are load-bearing: - Userinfo confusion -- a loopback literal in the userinfo position in front of a hostile authority. The real host is the hostile one. Already correct via `url.Parse().Hostname()`; now pinned so nobody switches to matching the raw URI, where the loopback prefix reads as trustworthy. - `http://0.0.0.0/cb` -- browsers commonly normalise this to loopback. We classify it remote, which fails closed; pinned so widening it stays a deliberate security decision. Oracle also asked for IPv6-literal and localhost-vs-literal coverage; both were already in the table.
…ns vets remote ones; docs: name both ways to supply the browser leg (#284) * docs: name both ways to supply the browser leg, not just the cookie resolver docs/cimd.md said the browser leg "needs a GET-capable PrincipalResolver, which ZeroID does not ship" and that the deployer "must register one that reads a session cookie". True as far as it goes, but it presents the harder route as the only route — and it is not the route Highflame itself takes. A deployer can instead front the browser leg ABOVE ZeroID: own the redirect, authenticate the human however they already do, then POST to /oauth2/authorize with a credential a form-based resolver reads — an RFC 7523 assertion signed by that surface, verified against its published JWKS. The browser never reaches the endpoint, so no GET-capable resolver is needed AND the CSRF exposure documented below does not arise: the caller is a server, not a navigation. That matters because the CSRF obligations are the expensive part of route 1, and a reader who thinks route 1 is mandatory takes them on unnecessarily. Highflame's own deployment is route 2 — Studio authenticates, mints an assertion and POSTs; AuthN's assertion resolver verifies it and ZeroID mints the code. Either way ZeroID stays the engine: it validates the CIMD document, enforces the redirect_uri allow-list, and issues the code. Route 1 is for deployers with no such surface of their own. Docs only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: route 2 relocates CSRF to the fronting surface; qualify the GET-resolver premise Review follow-up (PR #284), both threads: - The bold premise said the browser leg needs a GET-capable resolver, full stop, while route 2 two paragraphs later needs none. It now says DIRECT browser access at /oauth2/authorize needs one, and the list is framed as the two ways to connect the browser leg. Same qualification on the 'ZeroID cannot detect this' paragraph: form-based-only is a misconfiguration only when no fronting surface exists. - Route 2 no longer claims the CSRF exposure 'does not arise'. Moving the final hop to a server-to-server POST removes navigation reachability of /oauth2/authorize itself, but an attacker can still navigate a victim to the fronting surface with an attacker-published client_id — so the CSRF-protected consent interaction must happen at that surface before the assertion is minted. Said so, explicitly. Also scopes the 'Highflame takes route 2' claim: MCP clients still go through Studio's local code-minting today; highflame-studio#1392 brings them onto this path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: route 1 gets ZeroID's login redirect, but not for a CIMD client #285 landed ErrPrincipalInteractionRequired + SetInteractiveLoginURL, so a cookie resolver no longer has to hand-roll the 302 to its login screen. It also refuses that redirect for a self-asserted (CIMD) client, which is the half that matters in this doc: on route 1 a CIMD authorization request only succeeds for a user who already has a session. * fix: cimd.allowed_domains actually restores redirects to a CIMD client three places document and no code implements. failAuthorize and redirectToInteractiveLogin both gated on client.SelfAsserted() alone, and RegistrationSource is set to "cimd" unconditionally at synthesis, so setting cimd.allowed_domains changed nothing. The handler never even received the allow-list — API carried only cimdEnabled. Both gates now go through one predicate, refusesRedirectTo, so the error redirect and the interactive-login redirect cannot drift apart: they answer the same question about the same client. Server.NewServer feeds it AllowedDomainCount() > 0 — the EFFECTIVE list, for the same reason the startup log uses it, since allowed_domains: [""] has length 1 and vets nothing. This is what makes the browser leg completable for an MCP CIMD client. An unvetted one is never sent to the login surface, so a user with no session cannot establish one and the flow cannot finish at all — the allow-list is the switch, and it was wired to nothing. The empty-allowlist startup warning now names that consequence too. Also ignores .gstack/, which is per-session browser audit output. * fix: hold CIMD redirect destinations to the allow-list too, not just publishers Review of the previous commit found the premise it rests on is not actually established. refusesRedirectTo reads "an allow-listed publisher is a vetted party, so redirects apply again" — but nothing tied redirect_uris to the allow-list, or even to the client_id host. synthesizeCIMDClient checks scheme rules only. So on any host where more than one party can publish a path — user content, a raw-file CDN, a broadly writable bucket, a shared internal app host, the config's own apps.acme.dev example — allow-listing it re-opened exactly what https://evil.example/cb, and an unauthenticated GET /oauth2/authorize 302s to evil.example. Worse than before the allow-list, in fact, because redirectToInteractiveLogin now walks a victim through the real login page first, so the code lands at the attacker after a genuine sign-in. redirectHostsAllowed closes it: an https redirect_uri must be on the client_id's own host or on the allow-list. Loopback and private-use schemes stay exempt — they deliver to the caller's own machine, which is the native and MCP client shape. In open mode domainAllowed admits everything, so this is a no-op there, correctly: open mode refuses those redirects outright. Also from review: - refusesRedirectTo refused to answer for a nil client by returning false. Folded the nil case in and dropped the duplicated check at both call sites, which is what "one predicate so they cannot drift" was supposed to mean. - CIMDConfig.AllowedDomains' godoc and zeroid.yaml still described the field as a fetch/SSRF lever with empty as a fine default. It now also decides whether a browser CIMD client can sign a user in at all, and both say so — along with the new obligation that listing a host asserts you vet who publishes there. - docs/cimd.md's "Errors are not redirected to a CIMD client" heading and a spec cross-reference pointing at §12.6 (Caching) instead of §12.7. * feat: a loopback CIMD callback is redirected to; the carve-out is about reach The §4.1.2.1 carve-out refuses to redirect to a self-asserted client because its redirect_uris are attacker-CHOSEN, which would make the endpoint "an unauthenticated redirector with the AS's own origin as the first hop." That is a claim about a REMOTE destination, and it was being applied to every destination. A 302 to 127.0.0.1 has no remote hop. The code lands on the machine the user is sitting at, and an attacker who can listen there already has local code execution — the same reasoning RFC 8252 §7.3 uses to accept loopback callbacks from clients nobody registered. CIMD does not weaken it. This is not a corner case, it is the MCP case. A CIMD client_id names the client VENDOR's domain, and the canonical document in docs/cimd.md — an ordinary desktop/CLI MCP client — lists loopback callbacks and nothing else. So the carve-out was costing the entire browser leg for the dominant client shape while preventing nothing, and cimd.allowed_domains was being asked to buy back something the loopback property already gives for free. refusesRedirectTo now asks whether the destination can reach a third party the client chose: local delivery proceeds, remote https stays subject to provenance and the allow-list. service.RedirectDeliversLocally holds the judgement, next to the URI rules it belongs with; exact-match loopback means 127.0.0.1.evil.com is remote, which is tested. Consequence worth stating: a desktop MCP client now completes the browser leg with no allowlist configured at all. Only clients with real https callbacks need cimd.allowed_domains, and the config surface, docs and spec §12.5 say so rather than the blanket MUST they carried an hour ago. * fix: pin the CIMD redirect invariants Oracle flagged as prose-only Addresses the Oracle review on #284. ## The vetted bypass depended on an untested call Once `cimd.allowed_domains` names a host, `refusesRedirectTo` stops refusing redirects for self-asserted clients deployment-wide, without re-checking the individual client's redirect host. That is only sound because resolution has already refused any document declaring an off-list https `redirect_uri` -- vetting the publication host alone does not cover it, since a document hosted on an allow-listed domain can declare `redirect_uris` pointing anywhere. `TestRedirectHostsAllowed` covered the predicate in isolation. Nothing covered the wiring, and the wiring is what can vanish: rebasing this branch onto the #312 singleflight refactor moved the fetch into `resolveUncached` and severed that call outright. It surfaced only because it happened to be a compile error. A refactor that left a same-named host variable in scope would have kept compiling while vetting the wrong host, and no test would have failed. `TestCIMDResolveClient_OffListRedirectURIRefused` drives `ResolveClient` end to end: a document served BY the allow-listed host declaring an off-list https redirect_uri must be refused, plus a control proving an allow-listed redirect host still resolves. Verified by mutation -- removing the `redirectHostsAllowed` call makes it fail with its own diagnostic. ## Two loopback classifications were unpinned `RedirectDeliversLocally` is the only gate for an unvetted self-asserted client, so its edge cases are load-bearing: - Userinfo confusion -- a loopback literal in the userinfo position in front of a hostile authority. The real host is the hostile one. Already correct via `url.Parse().Hostname()`; now pinned so nobody switches to matching the raw URI, where the loopback prefix reads as trustworthy. - `http://0.0.0.0/cb` -- browsers commonly normalise this to loopback. We classify it remote, which fails closed; pinned so widening it stays a deliberate security decision. Oracle also asked for IPv6-literal and localhost-vs-literal coverage; both were already in the table. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Yash Datta <yd2590@columbia.edu>
3 participants
What
Collapses concurrent CIMD document fetches for the same
client_idinto a single outbound fetch.The gap
The resolution cache only helps once a fetch has completed. Until then every arriving request is a miss and starts its own fetch — so N simultaneous first-time requests for one
client_idmeant N DNS resolutions, N TLS handshakes, and N × up-to-5s of request occupancy, for a document that is byte-identical every time.That multiplier was free to the caller and required no credential: client resolution runs ahead of the
PrincipalResolverchain (#285), so an unauthenticated request reaches the fetch. Concurrency was the only input needed.ResolveClientnow usesgolang.org/x/sync/singleflight; the fetch/validate/synthesize/cache body moves toresolveUncachedso the callback stays readable.Two details that are easy to get wrong
*domain.OAuthClient— the same reasoncachedResultalready returns a copy. Sharing one instance would let any waiter mutate what the others hold.Scope — what this does not fix
Stated plainly because it's easy to over-read: this bounds duplicate concurrent work for one
client_id. It does not bound distinct-URL abuse. A caller cycling unique paths gets a fresh flight each time, misses the cache, walks past negative caching (which is per-URL), and churns the 1000-entry cache's eviction.Only an edge rate limit closes that.
docs/cimd.mdnow says so explicitly rather than as a trailing aside, and separates what each control actually bounds: the caps bound one fetch, coalescing bounds duplicate concurrent work,allowed_domainsbounds who can aim it.Tracked on the deployer side in highflame-cloud#2358.
Verification
concurrent resolutions performed 8 fetches, want 1. A coalescing test that would pass anyway is worth nothing, so this matters more than the green run.*domain.OAuthClientvalues, and that mutating one caller'sRedirectURIsdoesn't change another's (catches a shallow clone).-race. Fullinternal/...unit suites green.golangci-lint: 0 issues.Context
Found while reviewing why CIMD is disabled in Highflame production. The two blockers were this and the missing edge rate limit; with both addressed, an open-ecosystem CIMD deployment is defensible.
🤖 Generated with Claude Code